Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

replace dep with go mod #3907

Merged
merged 12 commits into from
Mar 18, 2019
Merged

replace dep with go mod #3907

merged 12 commits into from
Mar 18, 2019

Conversation

alessio
Copy link
Contributor

@alessio alessio commented Mar 15, 2019

Closes: #3919 #3630

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote tests
  • Updated relevant documentation (docs/)
  • Added a relevant changelog entry: sdkch add [section] [stanza] [message]
  • rereviewed Files changed in the github PR explorer

For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@alessio
Copy link
Contributor Author

alessio commented Mar 15, 2019

@codecov
Copy link

codecov bot commented Mar 15, 2019

Codecov Report

Merging #3907 into develop will decrease coverage by 0.01%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #3907      +/-   ##
===========================================
- Coverage    60.26%   60.25%   -0.02%     
===========================================
  Files          196      196              
  Lines        14615    14615              
===========================================
- Hits          8808     8806       -2     
- Misses        5214     5216       +2     
  Partials       593      593

Copy link
Member

@jackzampolin jackzampolin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This gets a big 👍, tACK, LGTM and also a comment that it reduces install time by ~3x

Makefile Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Copy link
Contributor

@liamsi liamsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a few comments which I think should be discussed & clarified. Also, it might be worthwhile to streamline this as much as possible across our repositories (tendermint/iavl/amino, ...).

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TestedACK

@fedekunze fedekunze mentioned this pull request Mar 17, 2019
4 tasks
@alessio alessio requested a review from jleni March 17, 2019 10:17
Replace sha1sum with jack's gosum and get rid of
vendor-deps.
Also don't compute hash on vendor/ contents.
Instead hash go.sum.

Disable unconvert lint check. It does not
work very well with go mod.

Remove update_vendor_deps once and for all.

Closes: #3919 #3630
@jleni
Copy link
Member

jleni commented Mar 17, 2019

This comment is more about the overall process and migration to go mod than this specific PR so I copy over here:

A risk I see with the migration to go mod is that we can have mismatches. For instance:

  • develop (gopkg.toml) vs this PR (go.mod)
  • old PR (gopkg.toml) and future develop (with new go.mod)
  • branches, etc.

git will not help much in these cases.

I would say, for the next few months, we should add a manual step to confirm that we don't make mistakes when mixing changes in old gopkg vs new go.mod files.

@cosmos cosmos deleted a comment from alessio Mar 17, 2019
@jleni
Copy link
Member

jleni commented Mar 17, 2019

@alessio I deleted my comment so I could move it over to the main thread and somehow it logs that I deleted yours, hehe :) Please repeat if I deleted something by mistake

@jleni
Copy link
Member

jleni commented Mar 17, 2019

it seems that something changed in the meantime :|

@alessio
Copy link
Contributor Author

alessio commented Mar 17, 2019

Checks should be back green shortly @jleni

@liamsi
Copy link
Contributor

liamsi commented Mar 17, 2019

Speaking of caching. Is there a particular reason to not cache the dependencies. This would probably speed up builds inside of circle ci:

      - restore_cache:
          keys:
            - go-mod-v1-{{ checksum "go.sum" }}

# ...

      - save_cache:
          key: go-mod-v1-{{ checksum "go.sum" }}
          paths:
            - "/go/pkg/mod"

https://circleci.com/blog/go-v1.11-modules-and-circleci/

@alessio
Copy link
Contributor Author

alessio commented Mar 17, 2019

@liamsi I agree with you, and although your example looks promising, my lack of skills in CircleCI pipelines design does not allow me to assess on the spot with enough confidence where I should drop those simple snippets. Things such as shortening build times fit into the category of (desired) optimisations, thus I'd focus on getting the main functionality of this PR over the line first. I'll ask our Experts (TM) @greg-szabo @mircea-c to take a look eventually 🙏️

Makefile Show resolved Hide resolved
@alessio
Copy link
Contributor Author

alessio commented Mar 17, 2019

All comments are addressed. I'm just waiting for at least one additional review to come through.

@@ -63,6 +64,7 @@ jobs:
name: binaries
command: |
export PATH="$GOBIN:$PATH"
make go-mod-cache
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also not a circcleci expert but line 62 reads - *dependencies. And the dependencies anchor is defined by:

deps: &dependencies
  run:
    name: dependencies
    command: |
      export PATH="$GOBIN:$PATH"
      make go-mod-cache

Shouldn't make go-mod-cache already have been called there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

@liamsi
Copy link
Contributor

liamsi commented Mar 18, 2019

It would be cool if this could serve as as a reference for other repo's (like tendermint etc). For this, I think we should:

We should probably merge this as is and address those in a follow-up PR though.

@mircea-c
Copy link

@liamsi I've created an issue in our repo to make the changes to CircleCi config. https://github.com/tendermint/devops/issues/207

@alessio alessio merged commit 6ce4d5e into develop Mar 18, 2019
@alessio alessio deleted the alessio/go-modules branch March 18, 2019 12:45
@dokwon
Copy link

dokwon commented Apr 4, 2019

Does this build even if the project directory is outside of GOPATH? having some issues aligning packages on my end...

@dokwon
Copy link

dokwon commented Apr 4, 2019

@jackzampolin @alessio

@fedekunze
Copy link
Collaborator

@dokwon I had the same issue, you need to remove the repository from $GOPATH

@alexanderbez
Copy link
Contributor

Strange. I can build the SDK both inside and outside my $GOPATH it just matters if you have your modules env set.

@jackzampolin
Copy link
Member

yeah its GO11MODULES=true or something like that.

@dokwon
Copy link

dokwon commented Apr 5, 2019 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants